libct/nsenter/nsenter_test.go: fix and improve #3158
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
thaJeztah
previously approved these changes
Aug 17, 2021
Member
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
left some comments/questions, but don't think they're blockers
kolyshkin
force-pushed
the
nsenter-tests
branch
from
06d98dc to
de7ec14
Compare
Contributor
Author
|
Rebased; renamed |
Contributor
Author
|
@AkihiroSuda PTAL |
Contributor
Author
|
Had to rebase after #3144 merge. @thaJeztah @AkihiroSuda PTAL |
kolyshkin
force-pushed
the
nsenter-tests
branch
from
de7ec14 to
940d66d
Compare
AkihiroSuda
reviewed
Sep 2, 2021
libcontainer/nsenter/nsenter_test.go
Outdated
|
|
||
| if err := cmd.Start(); err != nil { | ||
| t.Fatalf("nsenter failed to start %v", err) | ||
| t.Fatal("nsenter failed to start:", err) |
Member
There was a problem hiding this comment.
Why? The previous form seems completely legit.
Contributor
Author
There was a problem hiding this comment.
A semicolon was missing. While adding it, I guess I decided to simplify it, that's it.
Contributor
Author
There was a problem hiding this comment.
The other reason is, I was using the same form in the subsequent patches, so I added it here, too, for uniformity.
Nevermind though, I re-did this commit so it uses %v in this and similar cases, and moved the patch to be the last one.
PTAL @AkihiroSuda
1. Make sure we close all file descriptors at the end of the test. 2. Make sure we close child fds after the start. 3. Use newPipe for logs as well, for simplicity and uniformity. Signed-off-by: Kir Kolyshkin <[email protected]>
The test was not working since at least commit 64bb59f renamed pid to stage2_pid (or maybe even earlier), so the pid was never received (i.e. pid.Pid was 0). The problem was not caught because os.FindProcess never return an error on Unix. Factor out and fix pid decode function: - use DisallowUnknownInput to get error if JSON will be changed; - check pids to make sure they are valid - and use unix.Wait4 to reap zombies. Signed-off-by: Kir Kolyshkin <[email protected]>
Instead of reading a single message, do read all the logs from the init, and use DisallowUnknownFields for stricter checking. While at it, use reapChildren to reap zombies (and add an extra check). Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
- add missing colons before error message; - unify error messages after cmd.Start and cmd.Wait, so that they show context and the error itself. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
nsenter-tests
branch
from
940d66d to
33dcb99
Compare
AkihiroSuda
approved these changes
Sep 3, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix and improve nsenter_test.go (originally part of #3114).